-
-
Notifications
You must be signed in to change notification settings - Fork 408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix in the cadc module in anticipation of coming changes to the servers #2326
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2326 +/- ##
==========================================
- Coverage 62.98% 62.93% -0.06%
==========================================
Files 131 131
Lines 17059 17084 +25
==========================================
+ Hits 10745 10752 +7
- Misses 6314 6332 +18
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
astroquery/cadc/core.py
Outdated
@@ -525,8 +525,11 @@ def get_data_urls(self, query_result, include_auxiliaries=False): | |||
urlencode({'ID': pid_sublist, | |||
'REQUEST': 'downloads-only'}, True))) | |||
for service_def in datalink: | |||
if service_def.semantics == 'http://www.openadc.org/caom2#pkg': | |||
# pkg is an alternative for downloading multiple | |||
if service_def.semantics in \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for line break, long lines up to ~110-120 is fine when they provide better readability
@@ -206,6 +206,7 @@ def test_authsession(self): | |||
|
|||
@pytest.mark.skipif(one_test, reason='One test mode') | |||
@pytest.mark.skipif(not pyvo_OK, reason='not pyvo_OK') | |||
@pytest.mark.skip('https://github.com/astropy/astroquery/issues/2325') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel leaving these as failing tests are better, it ensures that they are not forgotten. Or maybe change them to xfail, so then we would notice easily if something got fixed upstream.
setup.cfg
Outdated
@@ -32,7 +32,7 @@ astropy_header = true | |||
text_file_format = rst | |||
xfail_strict = true | |||
remote_data_strict = true | |||
addopts = --doctest-rst | |||
#addopts = --doctest-rst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has to stay, we switched on testing the docs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll fix my comment before merging.
Locally I run into timeouts (I suspect my IP might got blacklisted), so merging it without ensuring that the remote tests are all OK. |
Thanks @andamian! |
Also had to comment out a few int tests with unrelated errors that require further investigation.
The patch is simple, has been tested and it's ready to go.